[default values] ReadBridge: mark bridge failures unrecoverable on Iceberg's retry path by cbb330 · Pull Request #668 · linkedin/openhouse

cbb330 · 2026-08-03T05:19:38Z

Summary

Important

Stacked on #645 — please review that first. Until it merges, the diff
here also shows its commits; the only commit belonging to this PR is
ReadBridge: mark bridge failures unrecoverable on Iceberg's retry path.
Once #645 merges this collapses to a small client-runtime + test diff. Draft until then.

loadMetadata is the loader BaseMetastoreTableOperations wraps in
Tasks.retry(20) with exponential backoff. That retry exists for the metadata
file read, which fails transiently — a network blip, a file not yet visible —
and can succeed on a later attempt.

Decode and apply are deterministic: a malformed config fails the same way every
attempt. Leaving those failures retryable therefore gave a deterministic error the
retry policy of a transient one:

21 attempts, ~87s of backoff (100 + 400 + 1600 + 5000×17 ms)
21 re-reads and re-parses of the metadata file from storage
to reproduce the error already available on the first attempt

Every reader of the table pays it simultaneously, so one malformed entry becomes
a read stall plus a storage read storm instead of a fast, clear error. The retry
loop also loses the real error: it reports whatever the last attempt threw.

Changes

Keep from / apply inside loadMetadata (decode before the file read so a
bad config never touches storage), and wrap IllegalStateException as Iceberg's
Tasks.UnrecoverableException. Iceberg already stops Tasks.retry on that type,
so no doRefresh decode field or AtomicReference<ReadBridge> is needed.

	when	retryable
`ReadBridge.from(config)`	before file IO in `loadMetadata`	no (`UnrecoverableException`)
metadata file read / parse	inside `loadMetadata`	yes
`bridge.apply(metadata)`	after file IO in `loadMetadata`	no (`UnrecoverableException`)

Testing Done

Manually Tested on local docker setup.
Added new tests for the changes made.
Updated existing tests to reflect the changes made.
No tests added or updated.
Some other form of testing like staging or soak time in production.

New regression test testMalformedConfigFailsBeforeTouchingStorage asserts a
Tasks.UnrecoverableException whose cause is IllegalStateException, and
verifyNoInteractions(mockFileIO) — failing before any storage access is the
property that matters.

JAVA_HOME=$(/usr/libexec/java_home -v 17) ./gradlew \
  :integrations:java:iceberg-1.2:openhouse-java-itest:test \
  --tests '*ReadBridge*' \
  --tests '*OpenHouseTableOperationsTest.testMalformed*' \
  --tests '*OpenHouseTableOperationsTest.testDoRefresh*'

Additional Information

Breaking Changes
Deprecations
Large PR broken into smaller PRs, and PR plan linked in the description.

Kept out of #645 deliberately, so that PR stays as reviewed and this defect is
reviewable on its own.

Downstream stack (rebased onto this tip): #674 → #675.

cbb330 · 2026-08-12T02:10:04Z

Stack update: this PR is step 2 of the read-bridge follow-ups (base retargeted onto #645).

[default values] Tables: apply server-supplied per-column defaults at read time #645 — substrate
This PR ([default values] ReadBridge: mark bridge failures unrecoverable on Iceberg's retry path #668) — decode off Iceberg retry path
[default values] Move read-bridge column-default ramp into OpenHouse #674 — policy/ramp in OpenHouse
apply follow-up — ReadBridge.apply (stacks on [default values] Move read-bridge column-default ramp into OpenHouse #674)

Overlay server-stamped column defaults onto every schema-id at metadata load using NestedField.initialDefault and a public TableMetadataParser JSON rebuild, keeping decode on the from()/apply() split from linkedin#668. Co-authored-by: Cursor <[email protected]>

loadMetadata is wrapped in Tasks.retry(20) for transient metadata *file* reads. Decode and apply are deterministic: a malformed config fails the same way every attempt, so retrying only burns ~90s and re-reads storage to reproduce an error already known on the first try. Keep from/apply inside loadMetadata (decode before IO so storage is never touched on a bad config), and wrap IllegalStateException as Tasks.UnrecoverableException — Iceberg already stops retry on that type. No doRefresh decode field or AtomicReference needed. Testing Done: - :integrations:java:iceberg-1.2:openhouse-java-itest:test --tests '*ReadBridge*' --tests '*OpenHouseTableOperationsTest.testMalformed*' --tests '*OpenHouseTableOperationsTest.testDoRefresh*'

Overlay server-stamped column defaults onto every schema-id at metadata load using NestedField.initialDefault and a public TableMetadataParser JSON rebuild, keeping decode on the from()/apply() split from #668.

Overlay server-stamped column defaults onto every schema-id at metadata load using NestedField.initialDefault and a public TableMetadataParser JSON rebuild, keeping decode on the from()/apply() split from linkedin#668.

mkuchenbecker · 2026-08-13T23:27:35Z

-        // The server encoder stamps an int field-id and a JsonNode value that round-trips through
-        // readTree, so reaching here means an encoder bug or transport corruption, not an expected
-        // state. Fail loud so it is caught, rather than silently reading NULL.
+        // Known keys are stamped as int field-id + JSON; anything else is a bug.


why did the comment change?

It was a style pass.

I made commit 57a9171 titled “Tighten read-bridge comments to why, not design essays.” The same message is on #674.

if you want, I can apply this "style" pass onto 645 so it doesn't look like a diff.

I didn't do that originally because I figured 645 is nearly done with review, so didn't want to touch it.

mkuchenbecker · 2026-08-13T23:27:47Z

+  Map<Integer, JsonNode> columnDefaults() {
+    return columnDefaults;
+  }
+


why did the java doc drop?

from a style pass, no behavior change. I wanted comments to not look like a wall of design text and be hard to review.

here is just a getter

mkuchenbecker · 2026-08-13T23:29:20Z

            .blockOptional();
-    // Capture the server-stamped per-table config so subclasses can gate read-time behavior via
-    // currentConfig(); absent => null. Side-channel only: never sent back on writes.
-    this.config.set(tableResponse.map(GetTableResponseBody::getConfig).orElse(null));


why did this drop?

The set moved. because we don’t bind on every GET anymore.

#645 wrote config as soon as the table-load response arrived, then called Iceberg. Iceberg can skip the metadata loader when the location is unchanged, and it UUID-checks after the loader. So, binding first meant currentConfig() could be a new stamp while current() was still the old metadata. It would mismatch config and metadata.

#668 binds only after a reload is actually accepted (config.set(fetched) after refreshFromMetadataLocation, and only if the loader ran). Skip-reload and a failed UUID check keep the old pair.

Iceberg skips the loader when tableLocation is unchanged, so writing config on every GET desyncs stamps from in-memory overlays. Bind after apply; skip-reload leaves the pair intact.

loadMetadata runs before Iceberg's UUID check. Setting config there desyncs stamps from current() if that check fails. Bind only after refreshFromMetadataLocation returns and the loader actually ran.

cbb330 changed the base branch from main to chbush/feature-flags-resolver August 12, 2026 02:09

This was referenced Aug 12, 2026

[default values] Move read-bridge column-default ramp into OpenHouse #674

Open

[default values] Implement ReadBridge.apply without an Iceberg fork #675

Closed

cbb330 force-pushed the chbush/read-bridge-decode-off-retry-path branch from 46d3b40 to 0f5e864 Compare August 12, 2026 02:24

cbb330 force-pushed the chbush/feature-flags-resolver branch from 3df3d5c to 86c62f6 Compare August 12, 2026 02:24

cbb330 changed the title ~~ReadBridge: decode the read-bridge config off Iceberg's retry path~~ [default values] ReadBridge: decode the read-bridge config off Iceberg's retry path Aug 12, 2026

cbb330 force-pushed the chbush/read-bridge-decode-off-retry-path branch from 0f5e864 to 99649c1 Compare August 12, 2026 04:53

cbb330 changed the title ~~[default values] ReadBridge: decode the read-bridge config off Iceberg's retry path~~ [default values] ReadBridge: mark bridge failures unrecoverable on Iceberg's retry path Aug 12, 2026

cbb330 marked this pull request as ready for review August 12, 2026 05:06

cbb330 force-pushed the chbush/read-bridge-decode-off-retry-path branch from 99649c1 to fbde8ee Compare August 12, 2026 16:51

cbb330 mentioned this pull request Aug 12, 2026

[default values] Handshake and drop read-bridge overlays on the server #678

Open

6 tasks

cbb330 mentioned this pull request Aug 12, 2026

[default values] Implement ReadBridge.apply without an Iceberg fork #679

Open

5 tasks

cbb330 mentioned this pull request Aug 13, 2026

[default values] Spark catalog itest for ReadBridge initial-default overlay #681

Open

17 tasks

mkuchenbecker reviewed Aug 13, 2026

View reviewed changes

cbb330 added 2 commits August 13, 2026 16:55

Tighten read-bridge comments to why, not design essays.

57a9171

Bind read-bridge config only when Iceberg reloads metadata.

7270ad8

Iceberg skips the loader when tableLocation is unchanged, so writing config on every GET desyncs stamps from in-memory overlays. Bind after apply; skip-reload leaves the pair intact.

Bind read-bridge config after Iceberg accepts the reload.

a9ab403

loadMetadata runs before Iceberg's UUID check. Setting config there desyncs stamps from current() if that check fails. Bind only after refreshFromMetadataLocation returns and the loader actually ran.

cbb330 force-pushed the chbush/read-bridge-decode-off-retry-path branch from cafff1b to a9ab403 Compare August 13, 2026 23:58

Navigation Menu

Uh oh!

[default values] ReadBridge: mark bridge failures unrecoverable on Iceberg's retry path - #668

[default values] ReadBridge: mark bridge failures unrecoverable on Iceberg's retry path#668
cbb330 wants to merge 4 commits into
linkedin:chbush/feature-flags-resolverfrom
cbb330:chbush/read-bridge-decode-off-retry-path

cbb330 commented Aug 3, 2026 •

edited

Loading

Uh oh!

cbb330 commented Aug 12, 2026

Uh oh!

mkuchenbecker Aug 13, 2026

Uh oh!

cbb330 Aug 14, 2026

Uh oh!

cbb330 Aug 14, 2026

Uh oh!

mkuchenbecker Aug 13, 2026

Uh oh!

cbb330 Aug 14, 2026

Uh oh!

mkuchenbecker Aug 13, 2026

Uh oh!

cbb330 Aug 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

cbb330 commented Aug 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Testing Done

Additional Information

Uh oh!

cbb330 commented Aug 12, 2026

Uh oh!

mkuchenbecker Aug 13, 2026

Choose a reason for hiding this comment

Uh oh!

cbb330 Aug 14, 2026

Choose a reason for hiding this comment

Uh oh!

cbb330 Aug 14, 2026

Choose a reason for hiding this comment

Uh oh!

mkuchenbecker Aug 13, 2026

Choose a reason for hiding this comment

Uh oh!

cbb330 Aug 14, 2026

Choose a reason for hiding this comment

Uh oh!

mkuchenbecker Aug 13, 2026

Choose a reason for hiding this comment

Uh oh!

cbb330 Aug 14, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cbb330 commented Aug 3, 2026 •

edited

Loading